Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change some notes into suggestions #42033

Merged
merged 3 commits into from
Jul 17, 2017
Merged

Change some notes into suggestions #42033

merged 3 commits into from
Jul 17, 2017

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented May 16, 2017

r? @petrochenkov since you commented on the same edits in #39458

@petrochenkov
Copy link
Contributor

meta: It's hard to read diagnostic differences in a test if they are done in the same commit that turns the test from compile-fail to ui. Could you move all the compile-fail -> ui conversions into a separate commit?

| ^^ not a type
|
= help: there is an enum variant `std::prelude::v1::Ok`,did you mean to use `std::prelude::v1`?
= help: there is an enum variant `std::prelude::v1::Result::Ok`,did you mean to use `std::prelude::v1::Result`?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, this test was moved to compile-fail due to instability in the order of suggestions (see #40775), so this will probably have to do be reverted, unless you want to fix the instability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix the instability

@@ -2,7 +2,7 @@ error[E0425]: cannot find function `baz` in this scope
--> $DIR/issue-14254.rs:29:9
|
29 | baz();
| ^^^ did you mean `self.baz(...)`?
| ^^^ help: did you mean `self.baz`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the "did you mean" questions... rhetoric?
Is it ok to omit question marks in this context, linguistically?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that we have no idea whether the suggestion author wrote try or did you mean, so we can't generate a questionmark out of thin air. We could cover the common cases like did you mean, but have you tried won't get caught or any other textual changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to add a heuristic catching "did you"/"have you" and appending the question mark.
All the messages are controlled by us after all, so they can always be tweaked if there are false positives/negatives.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm assuming adding an extra argument to span_suggestion would be cumbersome, but it's a variant too.)

Copy link
Contributor Author

@oli-obk oli-obk May 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could allow a placeholder in the message. So if you write did you mean {}? it will produce

did you mean `foobar`?

An in case it's not displayed inline, but as a help, the placeholder is replaced by "the following":

did you mean the following?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would probably be the most convenient (I can live with a bit of home-grown formatting).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I implemented this, but the code complexity goes way up. I need to find the correct {}. I'll also need to allow {{ to escape opening brackets. I need to allow some way to specify a replacement like "the following", ....

How about my first variant suggested here?

add a new enum so the lint author can choose a message

enum SuggestionMsg {
    DidYouMean,
    Try,
    Custom {
        label: String, // msg to display as label
        help: String, // msg to display as help
    },
}

cc @nrc this touches on prepending help: before suggestions displayed as labels

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through all suggestions' messages in rustc:

  • 5 "did you mean {the following}"
  • "try casting to a reference instead:"
  • "try casting to a Box instead:"
  • "possible candidates found in another module, you can import them into scope:"
  • "did you mean to use the variant's enum"
  • "try adding parentheses:"
  • "try parenthesizing the first index"
  • "try placing this code inside a block"
  • "make this visible only to module {} with in:"
  • really long message about to_owned
  • "to access tuple elements, use"
  • "to force the closure to take ownership of {} (and any other referenced variables), use the move keyword, as shown"
  • "consider using a reference instead"

I don't see a good way to get all of these under one hat.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ISTM that the client should choose the punctuation here, which suggests that using {} is the best way. However, the extra complexity does seem a bit annoying. I think the current mix of template and free text is probably wrong - could we either allow totally free text or lock down the structure of the text more strictly (as an aside, I've never liked the "did you mean ..." phrasing, feels patronising, I'd prefer "you could try ...", but I don't care too much and this is a bit orthogonal.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll create a commit with the enum variant to see how it looks. Then we can reiterate.

@arielb1 arielb1 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 16, 2017
@bors
Copy link
Contributor

bors commented May 17, 2017

☔ The latest upstream changes (presumably #41907) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk force-pushed the suggestions branch 2 times, most recently from 3a5d066 to e1974c5 Compare May 18, 2017 15:27
/// * should not contain any parts like "the following", "as shown"
/// * may look like "to do xyz, use" or "to do xyz, use abc"
/// * may contain a name of a function, variable or type, but not whole expressions
///
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some guidelines for suggestion messages. This way we can keep appending the suggestion at the end. I like this solution, because it's easy to read on the command line, requires little effort from suggestion authors and works well with non-label suggestions and json output.

@petrochenkov
Copy link
Contributor

Is this ready for re-review?
Everything seems ok, but Travis is failing on UI test ui/cast-to-unsized-trait-object-suggestion.rs.

@@ -35,46 +35,29 @@ macro_rules! attr_proc_mac {
}

#[derive(FooWithLongNan)]
//~^ ERROR cannot find derive macro `FooWithLongNan` in this scope
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to keep these annotations.
UI and compile-fail tests are going to be unified and ERROR/WARN will be necessary again.

@petrochenkov
Copy link
Contributor

cc @nrc this touches on prepending help: before suggestions displayed as labels

I'd vote for no "help: ", especially for messages for labels that previously didn't had it

@oli-obk
Copy link
Contributor Author

oli-obk commented May 22, 2017

Is this ready for re-review?

yes

Everything seems ok, but Travis is failing on UI test ui/cast-to-unsized-trait-object-suggestion.rs.

fixed

I think it's better to keep these annotations.
UI and compile-fail tests are going to be unified and ERROR/WARN will be necessary again.

done

I'd vote for no "help:", especially for messages for labels that previously didn't had it

As long as the message makes it clear that it's a suggestion... which all suggestions' messages should, but as discussed in the linked PR, this requires suggestion authors to take care when formulating the message.

@petrochenkov
Copy link
Contributor

@bors r+

help: try: `x` looks horrible, though

@bors
Copy link
Contributor

bors commented May 23, 2017

📌 Commit 72eb010 has been approved by petrochenkov

@oli-obk
Copy link
Contributor Author

oli-obk commented May 23, 2017

help: try: x looks horrible, though

I can create a PR that recognizes sentences starting with "try" or "you can try" and then leave out the "help:"

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 23, 2017
Change some notes into suggestions

r? @petrochenkov since you commented on the same edits in rust-lang#39458
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 23, 2017
Change some notes into suggestions

r? @petrochenkov since you commented on the same edits in rust-lang#39458
@arielb1 arielb1 added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 23, 2017
@Mark-Simulacrum
Copy link
Member

I'm uncertain, but it looks like this caused the PR failure, but this is odd, since the checks here passed. It might be a conflict with another PR. Here's the relevant log: https://gist.github.com/Mark-Simulacrum/b5b140d6e27e99ace2598be544d6c405 which says that the file moved in this PR from compile-fail to ui tests failed UI testing. I haven't investigated beyond this.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 24, 2017
Change some notes into suggestions

r? @petrochenkov since you commented on the same edits in rust-lang#39458
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 28, 2017

I need a decision whether it's fine to completely refactor error reporting in resolve by storing all the errors in a table and report them after resolve is done.

@petrochenkov
Copy link
Contributor

@oli-obk
Or you can move the problematic test from ui back into compile-fail.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 28, 2017

I like fixing broken things. I'm sure this is not the only situation, just the only one we're testing. Also the paths reported are rather suboptimal, since no newcomer should have to see std::prelude::v1::...

@bors
Copy link
Contributor

bors commented Jun 28, 2017

☔ The latest upstream changes (presumably #42850) made this pull request unmergeable. Please resolve the merge conflicts.

@aidanhs
Copy link
Member

aidanhs commented Jul 5, 2017

I'm going to mark this as being with the compiler team, cc @rust-lang/compiler, to get a response to

I need a decision whether it's fine to completely refactor error reporting in resolve by storing all the errors in a table and report them after resolve is done.

so this PR can progress in some way (either by moving the test back to compile-fail for this PR, or by going the proposed route, or something else).

@aidanhs aidanhs added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 5, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jul 11, 2017

Let's talk about this at the compiler meeting thursday?

The produced paths aren't stable between builds, since
reporting paths inside resolve, before resolve is finished
might produce paths resolved to type aliases instead of
the concrete type.

Compile-fail tests can match just parts of messages, so they
don't "suffer" from this issue.

This is just a workaround, the instability should be fixed
in the future.
@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 17, 2017

I moved the test back to compile-fail and will produce a new PR in the future to fix the instability.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 17, 2017

📌 Commit eb7f429 has been approved by petrochenkov

@bors
Copy link
Contributor

bors commented Jul 17, 2017

⌛ Testing commit eb7f429 with merge 5803f99...

bors added a commit that referenced this pull request Jul 17, 2017
Change some notes into suggestions

r? @petrochenkov since you commented on the same edits in #39458
@@ -16,7 +16,7 @@ error[E0308]: mismatched types
31 | if (x = x) {
| ^^^^^^^
| |
| help: did you mean to compare equality? `x == x`
| help: did you mean to compare equality?: `x == x`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these look... funny to me now. I think it's just the juxtaposition of ? and :... can we end it with something like "did you mean to compare equality? Consider: x == x" instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... those came in after a rebase. I'll create a new PR fixing up all suggestions that came in after I wrote the guidelines into the docs.

@carols10cents carols10cents added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 17, 2017
@bors
Copy link
Contributor

bors commented Jul 17, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 5803f99 to master...

@bors bors merged commit eb7f429 into rust-lang:master Jul 17, 2017
bors added a commit that referenced this pull request Jul 23, 2017
bors added a commit that referenced this pull request Aug 1, 2017
resolve: Try to fix instability in import suggestions

cc #42033

`lookup_import_candidates` walks module graph in DFS order and skips modules that were already visited (which is correct because there can be cycles).
However it means that if we visited `std::prelude::v1::Result::Ok` first, we will never visit `std::result::Result::Ok` because `Result` will be skipped as already visited (note: enums are also modules here), and otherwise, if we visited `std::result::Result::Ok` first, we will never get to `std::prelude::v1::Result::Ok`.
What child module of `std` (`prelude` or `result`) we will visit first, depends on randomized hashing, so we have instability in diagnostics.

With this patch modules' children are visited in stable order in `lookup_import_candidates`, this should fix the issue, but let's see what Travis will say.

r? @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.